Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding EQDSK support #391

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fusionby2030
Copy link

A draft for EQDSK support by modifying the geometry, geometry_loader, and build_sim files.

A corresponding geometry file is contributed to data/third_party/geo (credit to Antti Salmi for the help in geometry file generations).

Additionally, an example configuration is made based off (copied from) the iter hybrid scenario and basic example already provided. But the PR can likely do without the eqdsk_example config... the example takes a long time to run (at least it runs :D)...

The flux surface averaging is done using contourpy and RectBivariate spline interpolation of the PSI 2D grid. Note, there appear to be systematic differences in the shaping parameters (R inboard, R outboard, triangularity).

Somewhere the requirements have to be updated to include the TSVV-15's eqdsk and contourpy

See #373 for more details and discussion.

… and build_sim files. The flux surface averaging is done using contourpy and RectBivariate spline interpolation of the PSI 2D grid. Note, there appear to be systematic differences in the shaping parameters (R inboard, R outboard, triangularity).
Copy link

google-cla bot commented Sep 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@tamaranorman tamaranorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR it looks really good and adds some nice functionality just a few small mainly formatting etc comments

torax/geometry.py Show resolved Hide resolved
torax/geometry.py Outdated Show resolved Hide resolved
torax/geometry.py Outdated Show resolved Hide resolved
torax/geometry_loader.py Outdated Show resolved Hide resolved
@@ -58,6 +59,11 @@ def _load_fbt_data(file_path: str) -> dict[str, np.ndarray]:
"""Loads the data from a FBT-LY file into a dictionary."""
return scipy.io.loadmat(file_path, squeeze_me=True)

from eqdsk import EQDSKInterface
def _load_eqdsk_data(file_path: str) -> dict[str, np.ndarray]:
eq = EQDSKInterface.from_file(file_path, no_cocos=True) # should probably handle COCOS shenanigans...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we need to handle here? Or maybe turn it into a formal todo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a formal TODO, not sure how the torax team wants to handle COCOS in the future.

from eqdsk import EQDSKInterface
def _load_eqdsk_data(file_path: str) -> dict[str, np.ndarray]:
eq = EQDSKInterface.from_file(file_path, no_cocos=True) # should probably handle COCOS shenanigans...
return eq.__dict__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use dict(eq) but also maybe rename to something like loaded_data or similar as eq is generally used for equals

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict(eq) did not work, therefore I have kept it as eq.__dict, except renaming as suggested.

# Gathering area for profiles
areas, volumes = [], []
R_inboard, R_outboard = [], []
flux_surf_avg_1_over_R2_eqdsk = [] # <1/R**2>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the underscore eqdsk is probably not necessary here?

To save vertical space etc is it worth preallocating the numpy arrays and then assigning in rather than needing to create the arrays and then turn them into arrays?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the underscore is useful as the from_chease method has similar syntax for the FSA quantities.

@tamaranorman
Copy link
Member

Can you also sign the CLA please.

We will need to do a bit of work internally to bring this in and the dependencies internally but will work on that in the meantime

@tamaranorman
Copy link
Member

Looks good, are you able to resolve the conflicts so that we can try it out


# ---- Interpolations
q_interp = scipy.interpolate.interp1d(psi_eqdsk_1dgrid, eqfile['qpsi'], kind='cubic')
psi_spline_fit = scipy.interpolate.RectBivariateSpline(X_1D, Z_1D, psi_eqdsk_2dgrid, kx=3, ky=3, s=0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, did you check robustness of the results to the hyperparameters of this method?


# set epsilon based on floating point precision of psi grid
# NOTE: this is hacky and should be replaced with a more robust method
epsilon = 1e-7
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can use eps from the constants.Constants dataclass

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this generated directly from the existing ITER_hybrid CHEASE file?

If so, and if it doesn't break the EQDSK file, would be good to mention this in a header

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if all else apart from geometry is identical to test_iterhybrid_predictorcorrector, then can shorten this file as well as making the provenance clearer, by copying the CONFIG from iterhybrid and then modifying it. See https://github.com/google-deepmind/torax/blob/main/torax/tests/test_data/test_iterhybrid_rampup_restart.py for an example

@jcitrin
Copy link
Collaborator

jcitrin commented Sep 27, 2024

Thanks, looks really good. Made some minor comments after another run through.

After these, and merging with main, we'll bring it in internally and push it from there, next week. Thanks again for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants